Overview

When GSSHWRES or GSINTRES were left blank, this would result in the boxes displaying 'x'. This was brought up in #1152, and some implementation discussion went on there. This is fixed by parsing the value for -W/-H and -w/-h using getGameScopeArg, and by refactoring getGameScopeArg to accommodate this.

This PR also flattens getGameScopeArg a bit as well, to reduce nesting and improve readibility.

Background

The reason for this bug is that we were only checking if the width and height flags (-w/-h/-W/-H) were defined, but not if they contained values. We incorrectly assumed that if the flags were present they would also have values, which is not the case.

As a result, we would fall into the else block when setting these resolutions and run GSINTRES="$(tr ' ' '\n' <<< "$GAMESCOPE_ARGS" | grep -wA1 "\-w" | tail -n1)x$(tr ' ' '\n' <<< "$GAMESCOPE_ARGS" | grep -wA1 "\-h" | tail -n1)" and GSSHWRES="$(tr ' ' '\n' <<< "$GAMESCOPE_ARGS" | grep -wA1 "\-W" | tail -n1)x$(tr ' ' '\n' <<< "$GAMESCOPE_ARGS" | grep -wA1 "\-H" | tail -n1)". However the logic to parse out the width and height would return garbage, because there was no numeric value to parse out. Instead, it would parse out the next flag after it, or if it was the last flag in the arguments list, it would just print that flag. So if we passed -W and -H, GSSHWRES would end up returning -Hx-H, because -H comes after -W, and -H is the last flag so for the height value it just returns itself.

This code snippet should illustrate the problem:

# This will return `-h`
GAMESCOPE_ARGS="-w -h"
tr ' ' '\n' <<< "$GAMESCOPE_ARGS" | grep -wA1 "\-w" | tail -n1)

The reason the dropdown would display x and not -Hx-H or -hx-h is because we have some logic to only display integers on this Yad UI element:

--field="$GUI_GSINTRES!$DESC_GSINTRES ('GSINTRES')":CBE "$(cleanDropDown "${GSINTRES//\"}" "$(printf "%s\n" "$("$XRANDR" --current | grep "[0-9]x" | awk '{print $1}' | grep "^[0-9]" | tr '\n' '!')")")" \
--field="$GUI_GSSHWRES!$DESC_GSSHWRES ('GSSHWRES')":CBE "$(cleanDropDown "${GSSHWRES//\"}" "$(printf "%s\n" "$("$XRANDR" --current | grep "[0-9]x" | awk '{print $1}' | grep "^[0-9]" | tr '\n' '!')")")" \

This is how we end up with x, that logic removes the -Hx-H/-hx-h.

Solution

The fix is to ensure that when we're parsing the resolution values, we should ignore any values that are not numeric. So if we get, for example, -H as our value, we should assume this is invalid and fall back to the default 1280x720. Even if one of the values is wrong, we should invalidate the entire resolution and default back.

In order to do this, I ended up with a messy solution that would be duplicated for both resolutions that we capture. A cleaner way would be to have a re-usable function, and ultimately that led me to deciding to use getGameScopeArg for this. Back when this was implemented during one of the GameScope refactors, I did not initially opt to use this for the GameScope resolutions as it was a bit tricky. However now it is possible and it worked out of the box by just using it as a drop-in replacement to capture the internal and show resolutions!

But sadly that victory was short lived and I found out getGameScopeArg has the same limitation as the existing logic, in that it also returned x (or actually, -hx-h/-Hx-H). It had the same behaviour as our existing logic. So getGameScopeArg needed a refactor.

The change I made was that when we're expecting getGameScopeArg to parse out a NUM value, we should check that the value we parse out is actually a number. Since this could be an integer value or a decimal value, we need to account for both. So we can use this regex to do so: grep -P "^([\d]+)(?:\.([\d]{1,2}?))?$". If we apply this regex to the garbage output of -w -h, which will return -h, we will get an empty string. When we get this we can default to returning the default value, which is specified as 1280 for width and 720 for height when we call getGameScopeArg.

# GameScope Show Resolution (corresponds to -W, -H options, uppercase) -- Actual GameScope window size (defaults to 1280x720) -- Dropdown
# Although this is a combobox, we still use "num" as the `getGameScopeArg` type because we want numeric validation
GSSHOWRESARGWIDTH="$( getGameScopeArg "$GAMESCOPE_ARGS" "-W" "$GSSHOWRESARGWIDTH" "" "1280" "num")"
GSSHOWRESARGHEIGHT="$( getGameScopeArg "$GAMESCOPE_ARGS" "-H" "$GSSHOWRESARGHEIGHT" "" "720" "num")"

GSSHWRES="${GSSHOWRESARGWIDTH}x${GSSHOWRESARGHEIGHT}"

# GameScope Internal Resolution (corresponds to -w, -h options, lowercase) -- Resolution that games see (defaults to 1280x720) -- Dropdown
# Although this is a combobox, we still use "num" as the `getGameScopeArg` type because we want numeric validation
GSINTRESARGWIDTH="$( getGameScopeArg "$GAMESCOPE_ARGS" "-W" "$GSINTRESARGWIDTH" "" "1280" "num")"
GSINTRESARGHEIGHT="$( getGameScopeArg "$GAMESCOPE_ARGS" "-H" "$GSINTRESARGHEIGHT" "" "720" "num")"

GSINTRES="${GSINTRESARGWIDTH}x${GSINTRESARGHEIGHT}"

This change allows us to correctly handle cases where the width and height flag are given, but do not return numeric values (resolution can only be numeric). Refactoring this in getGameScopeArg allows us to re-use this function for setting the GameScope resolutions finally and also means anywhere else in the GameScope logic that may have been impacted by this will also benefit from this fix. (I don't think anywhere else was impacted, but it is still good to have this logic contained in one place).

Remaining Work

Since this is a change to getGameScopeArg we need to make sure this fix works correctly. I did some testing before opening a PR and values for comboboxes and numeric values seem to still save correctly, but this will need extensive testing before it can be merged.

Future Work

GameScope does allow you to simply pass the -h or -H flags, and it will calculate a 16:9 resolution based on the height alone (GameScope assumes 16:9 if not specified, which is why I am proposing that we default to 16:9 as well, to match GameScope behaviour). Perhaps in future in SteamTinkerLaunch we can check if we have a height value, and calculate an appropriate width if it is blank. We could do this by checking if [ -z "${GSINTRESARGWIDTH}" ] && [ -n "${GSINTRESARGHEIGHT}" ]" and then assigning a width value to GSINTRESARGWIDTH`.

This would really only be useful for users that manually enter a GameScope arguments string into the Game Menu or their Per-Game Config File, as -h 1080 -- is valid and GameScope would recognise this, but SteamTinkerLaunch would not.

This potential future work would not really apply to the GameScope menu and would probably not benefit it in any significant way, unless x1080 would actually parse out the height and store it but not the width. We could consider this in future though I suppose.

The remaining work here is to test this thoroughly. It fixes the bug it set out to fix but we need to regression test :-)